feat(mailserver): stop reconciling history if we were online - #7673
feat(mailserver): stop reconciling history if we were online#7673jrainville wants to merge 4 commits into
Conversation
|
There was a problem hiding this comment.
Pull request overview
This PR refines mailserver (store node) history reconciliation so that the protocol only queries history for periods where live delivery may have been missed (offline/unreliable windows), while tracking a monotonic per-topic “known complete through” cursor and checkpointing it during reliably connected full-node operation.
Changes:
- Introduces typed reconciliation windows (
HistoryReconcileWindow) and updates the Waku → Transport → Messaging API surface accordingly. - Reworks Messenger historic sync into a queued, bounded-window worker that coalesces/retries reconciliation requests without expanding into reliably-online time.
- Updates mailserver topic cursor persistence semantics (0 = uninitialized; monotonic advancement) and adds unit coverage around windowing, coalescing, pause/resume, and cursor checkpointing.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| services/mailservers/database.go | Makes topic upserts monotonic and adds AdvanceLastRequest to checkpoint initialized topic cursors. |
| services/mailservers/api_test.go | Extends DB tests for cursor advancement and non-regression behavior. |
| protocol/messenger.go | Adjusts pause/resume and connectivity handling to enqueue bounded reconciliation windows and checkpoint cursors. |
| protocol/messenger_settings.go | Wakes the historic sync worker when mobile-network syncing is enabled. |
| protocol/messenger_pause_play_test.go | Adds coverage ensuring resume enqueues a single bounded catch-up window. |
| protocol/messenger_mailserver.go | Adds window-aware historic sync options, bounds reconciliation queries, and makes cursor updates monotonic. |
| protocol/messenger_mailserver_window_test.go | New tests for window flooring, window translation, and concurrent/coalesced queue behavior. |
| protocol/messenger_mailserver_test.go | Adds coverage that community-description fetch respects reconciliation window bounds. |
| protocol/messenger_mailserver_cycle.go | Implements queued historic sync requests, coalescing, retry/defer semantics, and periodic cursor checkpointing. |
| pkg/messaging/waku/types/waku.go | Updates the Waku interface to emit typed reconcile windows and report delivery reliability. |
| pkg/messaging/waku/types/connection.go | Defines the HistoryReconcileWindow type. |
| pkg/messaging/waku/history_reconcile.go | Emits unreliable-delivery windows instead of a level-trigger signal; adds reliability tracking and HistoryDeliveryReliable. |
| pkg/messaging/waku/history_reconcile_test.go | Adds tests for window tracking bounds/disjointness and light-node reliability behavior. |
| pkg/messaging/waku/gowaku.go | Updates the reconcile channel type/buffering to carry windows. |
| pkg/messaging/types/connection_status.go | Re-exports HistoryReconcileWindow from the waku layer. |
| pkg/messaging/layers/transport/transport.go | Surfaces typed reconcile windows and delivery-reliability reporting from transport. |
| pkg/messaging/api.go | Exposes typed reconcile windows and delivery-reliability reporting on the messaging API. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| select { | ||
| case w.historyReconcileNeeded <- struct{}{}: | ||
| default: // a signal is already pending; coalesce | ||
| case w.historyReconcileNeeded <- window: | ||
| case <-w.ctx.Done(): | ||
| } |
Jenkins BuildsClick to see older builds (36)
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## fix/sds-being-off-after-offline #7673 +/- ##
====================================================================
+ Coverage 42.23% 62.56% +20.33%
====================================================================
Files 854 870 +16
Lines 117902 121369 +3467
====================================================================
+ Hits 49795 75938 +26143
+ Misses 61690 37691 -23999
- Partials 6417 7740 +1323
Flags with carried forward coverage won't be shown. Click here to find out more.
|
354b595 to
5a39ab5
Compare
|
@igor-sirotin reminder to review please |
| // dropped — the worker spaces, serializes and retries them. | ||
| func (m *Messenger) checkpointHistoryWatermarks(through time.Time) { | ||
| if through.IsZero() || m.mailserversDatabase == nil || m.messaging == nil || | ||
| !m.messaging.HistoryDeliveryReliable() { |
There was a problem hiding this comment.
Consider this:
- I start as
LightClient - Have a stable connection for 10h
- Lose a connection for 5min
- --> Last 10 hours will be fetched
It seems that this is what you intended, but why?
We discussed making store node reconcillation more aggressive for light client, but I remember we abandoned this idea in the end, because with some bugfixes light client became more reliable.
| // historic-sync worker (startHistoricSyncWorker). Non-blocking; concurrent | ||
| // triggers coalesce into a single pending sync and are never silently | ||
| // dropped — the worker spaces, serializes and retries them. | ||
| func (m *Messenger) checkpointHistoryWatermarks(through time.Time) { |
There was a problem hiding this comment.
checkpoint, watermark, last_request, "complete through", completeness cursor, history sync, history reconcile, gaps... 😰
The terminology is overloaded and confusing now.
I literally had to ask Claude to understand "checkpoint" vs "watermark" 😄
Maybe:
- watermarks => cursor/mark
- checkpoint => monitor
Claude also suggests "coverage", I like it too.
Up to you, just feedback
| m.historicSyncMu.Lock() | ||
| if m.historicSyncInFlight { | ||
| m.historicSyncMu.Unlock() | ||
| m.logger.Debug("defer historic sync request (already in progress)") | ||
| return nil, false, nil | ||
| } | ||
| m.historicSyncInFlight = true | ||
| m.historicSyncMu.Unlock() | ||
| defer func() { | ||
| m.historicSyncMu.Lock() | ||
| m.historicSyncInFlight = false | ||
| m.historicSyncMu.Unlock() | ||
| m.notifyHistoricSyncWorker() | ||
| }() | ||
|
|
||
| response, err := func() (*MessengerResponse, error) { |
There was a problem hiding this comment.
What was wrong with withHistoricSyncInFlight? 🤔
| @@ -580,8 +588,12 @@ func (m *Messenger) SetPaused(paused bool) { | |||
| if m.httpServer != nil { | |||
| m.httpServer.ToForeground() | |||
| } | |||
| if m.started { | |||
| m.asyncRequestAllHistoricMessages() | |||
| if m.started && wasPaused { | |||
| pausedAt := m.historyPausedAt.Swap(0) | |||
| from := time.Unix(pausedAt, 0) | |||
| if pausedAt > 0 && from.Before(now) { | |||
| m.asyncRequestHistoricMessages(types2.HistoryReconcileWindow{From: from, To: now}) | |||
| } | |||
There was a problem hiding this comment.
Seems to me that historyPausedAt custom logic shouldn't be needed. We set the last watermark when pausing, so when unpausing the next checkpointWatermark should kick in and do fill the empty window.
Actually, why do we need to asyncRequestAllHistoricMessages when unpausing at all? Don't we keep Transport running when the app is in background? Otherwise how do Andorid "native" push notifications work?
There was a problem hiding this comment.
Good question. I can try to remove it in a new commit. I'll also remove the guards that block taking watermarks when paused, so as long as we have peers in the background, it will behave the same as in the foreground.
I'll run it through the mobile e2e and see if it still works
There was a problem hiding this comment.
@jrainville actually, perhaps even history gaps filling could still make sense in background
There was a problem hiding this comment.
I think we decided to pause them because they are more intensive, so we wanted to avoid using too much memory while the phone is backgrounded. If it uses too much, the Android system could decide to kill it
There was a problem hiding this comment.
They could have been more intensive indeed, but perhaps with the old implementation. I don't see why a single store query would consume more resources than Filter connection with 3 remote peers. I'd give it a try, anyway it should only kick in when a connection is lost, not constantly syncing.
There was a problem hiding this comment.
True. I'll do it in a separate ticket
| func (m *Messenger) historicSyncNow() time.Time { | ||
| if m.messaging == nil { | ||
| return time.Now() | ||
| } | ||
| return m.calculateMailserverTo() | ||
| } |
There was a problem hiding this comment.
What's this about? I'm not following the logic 🤔
| if !m.isPaused() && newState { | ||
| switch { | ||
| case !wasOnline && !offlineSince.IsZero() && offlineSince.Before(historyNow): | ||
| m.asyncRequestHistoricMessages(types2.HistoryReconcileWindow{From: offlineSince, To: historyNow}) | ||
| offlineSince = time.Time{} | ||
| case wasOnline && force && previousCheck.Before(historyNow): | ||
| // The process did not observe connectivity during this interval | ||
| // (typically system sleep), so treat it as unreliable. | ||
| m.asyncRequestHistoricMessages(types2.HistoryReconcileWindow{From: previousCheck, To: historyNow}) | ||
| } | ||
| m.notifyHistoricSyncWorker() | ||
| } |
There was a problem hiding this comment.
Wouldn't this be done on HistoryReconcileNeeded now?
There was a problem hiding this comment.
Good question, TLDR: it's not the same type of checks and removing this could miss fetching when coming from offline
Long answer:
Not completely. This block is a separate producer of reconciliation windows.
history_reconcile handles transport-level reliability:
- Degraded relay mesh while the process remains online.
- Recovery after the transport becomes unreliable.
- It may miss system sleep if Waku still reports
Connected.
This block handles app-level gaps that transport may not observe:
- Explicit offline → online transitions.
- Long gaps between checks, typically system sleep or process suspension.
Both paths enqueue bounded windows, and coalesceHistoricSyncRequests merges overlapping requests, so keeping this block is harmless when both detect the same outage. Removing it would risk missing sleep or app-level connectivity gaps that Waku never reports as unreliable. The notifyHistoricSyncWorker() call only wakes the worker; it does not duplicate the reconciliation detection.
There was a problem hiding this comment.
I see, thanks. Eventually it would be nice to put this logic into Transport.
Well, this loop relies on m.Online(), which == m.transport.connectionState != ConnectionStateDisconnected. So it's effectively exactly the same thing as HistoryReconcileNeeded relies on.
It may miss system sleep if Waku still reports
Connected
This is a good note though, I'm not sure how quickly Waku connection status reacts to connection status change, is it possible to miss to store-fetch some messages because we detect the connection loss with a delay? 🤔
5a39ab5 to
e1ce65d
Compare
|
Thanks a lot for the review @igor-sirotin Very good points you raised. I address the small things in the "code review updates" commits and the bigger change of not reconciling mobile in the background in continue tracking history cursors when backgrounded. Please review again. Meanwhile, I'll do some tests on the status-app to see if it behaves correctly still. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
services/mailservers/api_test.go:68
- These two assertions duplicate the previous ContentTopic/LastRequest checks for topics[0], which makes the test noisier without adding coverage.
require.Equal(t, topics[0].ContentTopic, topicA)
require.Equal(t, topics[0].LastRequest, 1)
| func (w *Waku) HistoryDeliveryReliable() bool { | ||
| return !w.cfg.IsLightClient() && w.reliablyConnected() | ||
| } |
There was a problem hiding this comment.
That's not true. The transport still works when paused. We should probably rename isPaused, it's even confusing the AI 😂
|
I think something broke in the latest version. If I go offline on my mobile, send messages on my desktop, wait, then go online again, my mobile doesn't fetch it. Even after restarting the app. It seems like window to fetch might be broken |
|
Pushed a new commit that should fix the problem I found above. Testing |
|
The issue still happens. I just realized I can reproduce on desktop too, so I'll test and fix there first, it's faster |
|
Ugh, the issue actually exists on develop too. I'll still fix here just because I don't want conflicts with this branch. |
|
Alright, I finally found the solution. The problem was not with mailserver, it was because when we go offline, we Stop the SDS manager and we never restart it. So any SDS message is never being processed until a restart. I'll add a new commit to fix it |
|
I fixed and put in another commit on top of develop. I confirm it works. I can also say that this PR makes store fetches way faster, because it fetches way less. I'll open a new PR and I'll rebase this branch on top of it |
031eb8d to
b12a742
Compare
|
Done, @igor-sirotin you can review again. |
Fixes #7568 ## Summary - bound automatic history reconciliation to observed unreliable-delivery windows - introduce typed reconciliation events carrying fixed `From` and `To` bounds - retain, retry, and coalesce pending windows while preserving disjoint outages - prevent stable online periods from being queried during later reconciliation - use `mailserver_topics.last_request` as a monotonic “known complete through” cursor - checkpoint initialized topic cursors during reliable full-node connectivity - schedule bounded reconciliation for offline recovery, sleep/wake, and pause/resume - remove duplicate startup fetching and retain one cursor-based startup catch-up - wake retained work when mobile-network syncing is enabled - bound newest-community-description queries to the reconciliation window - preserve existing behavior for initial topic history, manual requests, and archive backfills ## Testing - added coverage for reliable and unreliable connectivity transitions - added overlapping/disjoint window coalescing and concurrent queue tests - added fixed retry-bound and blocked-work retention coverage - added cursor initialization, checkpointing, and monotonic update tests - added pause/resume and community-description window tests - ran focused Waku and protocol tests, including race coverage - ran messaging package compilation and `go vet` - ran protocol tests with `-tags lint` because `libsds.h` is unavailable in this checkout
Continues tracking for history reconciliation and taking cursors even when backgrounded. Then only when back online does it trigger the fetch from the last stable point.
b12a742 to
5d2b905
Compare
|
I tested on my mobile too and everything works as expected now |
Fixes #7568
Status-app PR: status-im/status-app#21714
Summary
FromandToboundsmailserver_topics.last_requestas a monotonic “known complete through” cursorTesting
go vet-tags lintbecauselibsds.his unavailable in this checkout